Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Rails 5.1.x #51

Merged
merged 1 commit into from
Oct 5, 2020
Merged

Support Rails 5.1.x #51

merged 1 commit into from
Oct 5, 2020

Conversation

brchristian
Copy link
Contributor

All of the currently released Solidus versions still support Rails 5.1.x, and I think it's important to try to continue to support it as long as possible in this extension in particular.

Many Solidus extensions rely on solidus_support v 0.5 and greater, and at the moment our store cannot get any of the latest commits from solidus_tax_cloud or solidus_handling_fees or solidus_stripe or solidus_related_products or . . . you get the idea . . . until we transition our application to Rails 5.2. (This is a huge undertaking, because it means somehow getting away from the deprecated solidus_active_shipping!)

Because this extension is so widely used (indeed that's the whole idea!), I believe it is important to have compatibility requirements that are as loose as possible, otherwise any requirements here will constrain the entire ecosystem of a user's Solidus extensions.

I'm not familiar with what if anything changed in activesupport from v5.1 to 5.2, but all of the specs are green and, as I say, all current versions of Solidus support 5.1 so I think we should endeavor to continue to do so until there is a convincing reason otherwise!

@aldesantis
Copy link
Member

@brchristian thanks for the PR! You're raising some valid concerns here.

I was thinking it may be an even better idea to remove the version constraints on activesupport entirely. Instead, let's rely on solidus_core etc. specifying the required Rails version.

What do you think?

@brchristian
Copy link
Contributor Author

@aldesantis I support this idea! I think that makes a lot of sense. And of course individual extensions can have constraints on the Rails version, too, if they make use of version-specific Rails features.

@brchristian
Copy link
Contributor Author

brchristian commented Aug 16, 2020

Hi @aldesantis I’ve gone ahead and done exactly as you suggested, removing the constraint on activesupport entirely. What do you think?

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if the commit message explained the conditional that's added here.

Rails 5.2 added the sqlite3.represent_boolean_as_integer
config variable. We omit setting this config in Rails < 5.2.
@brchristian
Copy link
Contributor Author

@jarednorman great idea, added it!

@brchristian
Copy link
Contributor Author

@aldesantis @jarednorman Anything I can do to help this along? We’ve been running this in our own fork in production for a few months now and would love to rejoin the party on the master branch!

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks @brchristian!

@aldesantis
Copy link
Member

@brchristian sorry, somehow I missed the notification for this one! LGTM, @jarednorman what do you think?

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me!

@aldesantis aldesantis merged commit 1011d52 into solidusio:master Oct 5, 2020
@brchristian brchristian deleted the rails5-1 branch October 5, 2020 16:22
@brchristian
Copy link
Contributor Author

Terrific, thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants